• fix(hot): split history shards by encoded size, not count #66 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[Claude Code]

Posting on James's behalf — follow-up to the size-based-splitting discussion.

Agree the size-based approach is the right call (the count-based alternative imposes ~10× metadata overhead on hot, dense addresses, which dominate real workloads). The objection is to the binary search, not the size budget.

Why the current splitter is more expensive than it needs to be

max_prefix_fitting does up to log₂(n) probes per shard, and each probe calls BlockNumberList::new_pre_sorted(&indices[..mid]) + serialized_size() — i.e., constructs a fresh RoaringTreemap from scratch per iteration. For a 2000-entry oversized merge that splits into ~13 shards, that's ~140 full roaring constructions on the overflow path. Not catastrophic (overflow is rare), but unnecessary.

Also, the if mid == 1 { break; } branch silently returns best = 1 (the initial value, never probed) without verifying that a 1-element list actually fits. Theoretical footgun.

Three cheaper heuristics

A. Incremental insert + exact size check (simplest). Insert indices one at a time into a working BlockNumberList; query serialized_size() after each. It's O(num_containers) per call, not O(n), so total work is O(n × containers) — for the dense path (1 container) effectively O(n); for the pathological all-distinct-containers path O(n²) ≈ 4M tiny ops, still cheaper than the current binary search's repeated full re-encodings. No probing, exact.

B. Boundary-triggered estimate (O(n)). serialized_size() only jumps when a new container is created or a container transitions representation (array↔bitmap↔run). Both are detectable from the input itself: (idx >> 16) changing (new container) or input crossing density thresholds. Maintain a running upper-bound estimate (treat every container as array: +12 B per new container, +2 B per element); emit when it crosses budget. Conservative — gives up some bitmap/run compression slack on dense paths — so periodically recalibrate with a real serialized_size() to recover it.

C. Segment-based greedy pack (O(n), exact). Pre-segment the merged input by (hi32, hi16) in one pass; each segment's encoded cost is predictable from cardinality. Greedy-pack segments into shards.

Recommendation: Option A

Smallest delta from the current PR. Replace max_prefix_fitting with a single-pass incremental builder:

let mut shard_start = 0;
let mut shard = BlockNumberList::default();
for (i, &idx) in all.iter().enumerate() {
    shard.push(idx)?;
    if shard.serialized_size() > ShardedKey::MAX_SHARD_BYTES {
        // emit prefix [shard_start..i) — rebuild once at the boundary
        let prefix = BlockNumberList::new_pre_sorted(all[shard_start..i].iter().copied());
        write_shard(all[i - 1], &prefix)?;
        shard_start = i;
        shard = BlockNumberList::default();
        shard.push(idx)?;
    }
}
write_shard(u64::MAX, &shard)?;

One roaring rebuild per shard boundary instead of log₂(n) per shard. Exact, no probing, simpler to read.

Orthogonal asks from your earlier review still stand

The four points from the change-request — characterise roaring encoding, add worst-case tests in storage-types, derive a documented safe upper bound, and move encoding-derived constants onto BlockNumberList — are independent of which splitting heuristic we use, and would land cleanly on top of Option A. The worst-case test in signet-storage-types is what would justify the MAX_SHARD_BYTES = 1500 number rigorously (right now it's asserted by comment, not by test).

[Claude Code]

Follow-up posted on James's behalf — layering observation, separable from the splitting-heuristic discussion above.

MAX_SHARD_BYTES = 1500 currently lives in signet-storage-types on ShardedKey, but that number is a property of MDBX's DUPSORT value cap (~1980 B on 4 KB pages, minus key2 and per-node overhead). Other backends that implement HotKv — MemKv today, hypothetical SQL/remote backends later — have no such limit, and forcing them to split into ~1500-byte shards just wastes work and inflates their on-disk/in-memory layout for no reason.

The dual-keyed table abstraction itself is fine; that's a deliberate primitive in this storage layer, not an MDBX-ism. The issue is purely that the byte budget is backend-specific while the splitting policy is generic.

Minimal cleanup that fixes the layering without touching the trait surface:

trait HotKvWrite {
    /// Maximum encoded size of a single stored value, in bytes.
    /// `None` means unbounded.
    fn max_value_bytes(&self) -> Option<usize> { None }
}

• signet-hot-mdbx overrides to return Some(1500) (or computes from page size).
• MemKv keeps the default None and writes one monolithic shard per address.
• append_to_sharded_history reads self.max_value_bytes(); if None, fast-path emit; otherwise run the size-based splitter against that budget.

This keeps the shared splitter (so we don't duplicate the algorithm per backend), keeps the schema shared (dual-keyed history tables remain the abstraction's primitive), and removes the MDBX-specific constant from signet-storage-types.

BlockNumberList::serialized_size() stays in signet-storage-types — it's a property of the encoding. Only the budget moves.

Happy to file this as its own issue and let ENG-2287 land first, or fold it into the same PR if it's small enough. Probably the former — different scope, easier to review separately.